-
Notifications
You must be signed in to change notification settings - Fork 895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Kafka messaging example #1799
Conversation
| Parent | | Span Prod1 | Span Rcv1 | | Span Prod2 | | ||
| Links | | | | Span Prod1 | | | ||
| Parent | | Span Prod1 | Span Rcv1 | Span Proc1 | Span Prod2 | | ||
| Links | | | | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already made a comment about this in the original PR at #1027 (comment) where @kenfinnigan then explained why a parent/child relationship might not always be possible or applicable.
Maybe we understand this problem better now that the first Kafka instrumentations based on these semantic conventions were already developed.
CC @anuraaga who might have an opinion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My views have evolved somewhat, but I think it's also colored by #65 not being resolved, and how the spans look when visualized.
While I still think it makes sense for producing a new message to have no parent on the "process" of the existing one, it really messes with the visualization aspect as it gives the impression, as I mentioned in #1085 (comment), that the production of a new message is a child of the incoming span.
I'm ok with adjusting things as it makes the visualization clearer. Maybe it is revisited when there is a concept of "follows from".
Thanks @trask for doing this. From re-reading it, I think my preference would be for the parent to be "Span Rcv1" and not "Span Proc1". The reason is that "Span Proc1" isn't the bit that produces the new message, it occurs in frameworks after the processing of the message is complete. So they're sequential operations, but not directly called one to the other. |
ah, I was assuming "Span Proc1" represents the entire processing of the message, e.g. a |
I think it still represents the entirety of the processing for a specific message, as in However, the creation of another message out after processing a message, at least in reactive messaging cases we have with Kafka and AMQP, is a separate step that runs once the message processing is complete. In essence, we create a reactive stream of "receive the incoming message" -> "process message" -> "create new outgoing message", with each step a separate component in the reactive stream |
@kenfinnigan just to clarify, it sounds like you agree with this change for non-reactive messaging flows? If so, maybe you could propose a separate example for reactive messaging flows? |
The proposed change is to the Kafka example, which would be applicable to reactive messaging flows. I don't remember enough about JMS to say whether what I think fits for the Kafka example is applicable to JMS and a Broker situation, or if a separate one is needed. |
oh, sorry, I think I confused things by mentioning how would you model |
I'm not super familiar with If a method had I might be wrong, but that's what it appears would be the case with an initial look |
I see, I was assuming the example was depicting the processing code sending the downstream message directly, as opposed to returning the message to be sent by the framework |
@trask you mean the example for Apache Kafka? If so, I think I was the one that originally added it and it was based on how messaging is usually handled with reactive streams in that the communication with Kafka is handled by the framework. After reviewing some traces the other day I realized what I'd proposed as the convention was a bit off, and should be adjusted |
thx, makes sense, I'll close this PR then since that was not my interpretation of the example |
I think(?) this was just an oversight in the example, and "Span Prod2" should parent "Span Proc1" (and no need for a link to "Span Prod1").